Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0008215opensim[REGION] OpenSim Corepublic2017-07-25 10:212017-07-31 16:38
Reportercolosi 
Assigned Tocolosi 
PriorityhighSeveritymajorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Versionmaster (dev code) 
Target VersionFixed in Versionmaster (dev code) 
Summary0008215: BuyLandPass will deliver land pass regardless of whether user pays
DescriptionThe flow implemented in recent commits to dev which implements the bulk of this in the LandManagemenModule calls the TriggerMoneyTransfer event to tell the money module to transfer the funds. It then delivers the pass (adds the user to the parcel access list with the expiration).

Unfortunately, the OnMoneyTransfer event doesn't return a value, so there is no way for the money module to tell the LandManagementModule whether or not the payment succeeded. If a user has insufficient funds, they can just keep buying passes without paying.

The ideal flow would be to supply the money module with a callback function to call once the transaction has succeeded or failed. This function could then cancel or provide access. Unfortunately, I don't think there is a way to do this using any of the current interfaces or events.

Here is my proposal for how we handle this:
We move the bulk of this implementation back into the money module, which can ensure that the pass is only granted if the payment succeeds. We move the bad actor checks (sessionID, parcel pass sales actually enabled, etc.) out of the LandManagementModule and into the handler. The BuyLandPass event should not get called if this was a bad actor or there is some error. It shouldn't be the responsibility of every event consumer to check this. Then, we let the money module handle the rest by consuming this event. I know it's ugly, but it is the only way I can see that doesn't break the interface and guarantees people get (and only get) what they pay for.

Now, for doing this in a better way, we spend a little time designing what interface we wish we had that we could run this through. We create that (or those) interface functions in MASTER, but don't use them yet. We comment that we will move this functionality here later, once 0.8 is deprecated. Down the road, when we finally deprecate support for 0.8, we change this event to use our new interface functions and we require each money module to update how it handles this transaction. Money Modules can now use this interface without having to fork their code, as they no longer have to support the versions of OpenSim which don't include these interface calls.
Steps To ReproduceAssuming you have a money module which implements this transaction type...
Click BuyPass on land when you don't have enough funds for the price. You'll be granted access and you won't be charged.
Additional InformationWe need a system for making long-term design changes without breaking backwards compatibility of addon modules. I suggest a deprecation schedule and an agreement that new interface functions will be added, but will not be used until the versions without those interface functions are deprecated.
TagsNo tags attached.
Git Revision or version number8739ceb00f34c618634983b25330a21f60eafe6d
Run Mode Grid (Multiple Regions per Sim)
Physics EngineBulletSim
EnvironmentUnknown
Mono VersionNone
ViewerFireStorm
Attached Files

- Relationships

-  Notes
(0032164)
colosi (reporter)
2017-07-25 16:43

There is another option. While we can't add new interface functions to the core and have addon modules consume them without having to fork their code to support various versions of OpenSim, I think we can add functions to the defined IMoneyModule interface which the core can call and implement them in the money module addons. For an old version of OpenSim, it will just see this as a function which is not in the interface and is never called, but it will not throw an error. For more recent versions of OpenSim, they'll use that interface function.

Plugh has proposed a new MoveMoney version which returns a bool and includes a MoneyTransactionType. I'd prefer we spend some more time thinking this through, but if we were to move forward and fix this flow keeping Ubit's code and adopting this new MoveMoney (or other new) interface function, then I'll propose a few modifications which will vastly improve this flow.

1. We don't just deliver the land pass when the money module function returns, even if it returns true. We supply the Money Module with a way to trigger the delivery.
2. We add an argument which is a callback function to the MM interface function. This is the function that the MM will call after it has attempted the transaction. We could design this callback either so it is called regardless of success or failure (in case it needs to do cleanup on failure) with an argument stating whether it succeeded or failed; or we could provide a separate callback function in the case of failure; or we could design this only to be called upon success, but that limits us in the future. This function will return a bool to let the MoneyModule know whether it succeeded or failed. This way, the money module can finalize the transaction or roll it back so that a user is never charged for something they didn't receive.
3. We add the scene as an argument to the MM interface function. We always want the scene which this event came from.
4. We add an argument which is a dictionary to the MM interface function args. Since we don't necessarily know what information we'll want about a type of transaction, this makes that extendible in the future. In the case of buying a land pass, we probably want to pass the parcel, the parcel land data, or specific parcel land data such as the name, description, UUID, location, and pass hours. That interface can change over time without breaking old versions since we can check for what is there and do nothing when parameters are missing.
5. The LandManagentModule will now have something like a DeliverLandPass function. Prior to delivery, this should re-check the LandData such as PassHours, PassPrice and UsePassList to ensure they haven't changed. This should return false if these have changed. If not, it should deliver access and return true (assuming it succeeded).
6. The inner component which just handles the granting access should be made a function which should also be called by the LSL script so that functionality is identical. This should not have the outer checking code because the script function would be supplying hours and pricing separate from the land settings.
(0032172)
colosi (reporter)
2017-07-26 10:15

Missed that Ubit is calling amountCovered before triggering the transaction. The current impl still isn't perfectly transactionally consistent, but it's much closer and is equivalent to some other purchase flows (like upload fees).

Sounds like Ubit is going to modify the MoveMoney iMoneyModule interface function to return a bool which will improve consistency further. Since delivery of land passes should rarely fail, there should be few cases where a refund needs to be applied. It's not as good as a callback from the MM which could roll back the transaction on delivery failure, but it's passable.

Further, it sounds like Ubit is going to add some additional data to the args. Waiting to see the final version of the new MoveMoney, but hopefully we've resolved this in some reasonable compromises.
(0032173)
kcozens (administrator)
2017-07-26 11:15

I have a new MoveMoney API call that I use in conjuction with a minor change to allow the ability to apply a charge to join a group. I haven't pushed the changes as I wasn't sure if someone was going to come after me for adding a new API call.
(0032229)
colosi (reporter)
2017-07-31 16:38

AmountCovered was being called already which reduces the likelihood that this could have easily been taken advantage of.

New iMoneyModule function was introduced which returns a bool. The granting of parcel access only happens if it returns true. It would be better to handle enacting of purchase delivery through a callback function supplied to the money module in the future, but this is acceptable in the meantime.

- Issue History
Date Modified Username Field Change
2017-07-25 10:21 colosi New Issue
2017-07-25 16:43 colosi Note Added: 0032164
2017-07-26 10:15 colosi Note Added: 0032172
2017-07-26 11:15 kcozens Note Added: 0032173
2017-07-31 16:38 colosi Note Added: 0032229
2017-07-31 16:38 colosi Status new => resolved
2017-07-31 16:38 colosi Fixed in Version => master (dev code)
2017-07-31 16:38 colosi Resolution open => fixed
2017-07-31 16:38 colosi Assigned To => colosi


Copyright © 2000 - 2012 MantisBT Group
Powered by Mantis Bugtracker